[PWGHF,Tools] Address Bug tracker issues in LcToPKPi-related files #14358
Conversation
|
O2 linter results: ❌ 36 errors, |
github-actions
bot
changed the title
lubynets
changed the title
|
Help wanted
|
|
| Configurable<double> maxR{"maxR", 200., "reject PCA's above this radius"}; | ||
| Configurable<double> maxDZIni{"maxDZIni", 4., "reject (if>0) PCA candidate if tracks DZ exceeds threshold"}; | ||
| Configurable<double> minParamChange{"minParamChange", 1.e-3, "stop iterations if largest change of any X is smaller than this"}; | ||
| Configurable<double> minRelChi2Change{"minRelChi2Change", 0.9, "stop iterations is chi2/chi2old > this"}; | ||
| Configurable<float> maxR{"maxR", 200., "reject PCA's above this radius"}; | ||
| Configurable<float> maxDZIni{"maxDZIni", 4., "reject (if>0) PCA candidate if tracks DZ exceeds threshold"}; | ||
| Configurable<float> minParamChange{"minParamChange", 1.e-3, "stop iterations if largest change of any X is smaller than this"}; | ||
| Configurable<float> minRelChi2Change{"minRelChi2Change", 0.9, "stop iterations is chi2/chi2old > this"}; |
There was a problem hiding this comment.
This will invalidate existing wagon configuration. Not sure we want that.
There was a problem hiding this comment.
So will it be a better solution to revert Configurables' types to double and static_cast to float functions' arguments?
There was a problem hiding this comment.
So, we define Configurables with double precision which we never use, since immediately cast it to float. Is it worth fixing once with taking care of wagons adjusting (also once, and the place of concern is well known)?
I will take the responsibility to wide-post in the HF chat the announcement and ask service wagons owners to do adjustment.
There was a problem hiding this comment.
OK, up to you (provided that @gluparel @xinyepeng agree).
There was a problem hiding this comment.
Dear @gluparel and @xinyepeng,
This PR changes the type of several Configurables from double to float in candidateCreator3Prong and candidateSelectorLc workflows. According to Vit's concern this may discard existing wagon settings.
I checked 15 HF service wagons containing "CandidateCreator3Prong" in their names - all of them have default values of changed Configurables. Thus no adjustment of numerical values of Configurables will be needed.
Also I did not find any HF service wagon with "candidateSelectorLc" in its name.
Therefore, the only place where wagon settings adjustment may be required is analyzers' custom wagons (if they use non-default values). If the PR is merged as is, I will notify analyzers in the HF chat.
Could you please approve or dis-approve such a change of Configurables types?
Hi @vkucera, thanks for your feedback! |
|
No, it should be instantiated as a member of a workflow UPD: Done (O.L.) |
lubynets
changed the title
|
Hi @vkucera, thanks for the feedback to the PR and comments. From my side I would keep it as it is now and undraft, but still some threads are unresolved. Can you comment on them (or resolve)? |
lubynets
requested review from
a team,
NicoleBastid,
alibuild,
apalasciano,
ddobrigk,
deepathoms,
dsekihat,
fcatalan92,
fcolamar,
fgrosa,
gluparel,
hahassan7,
iarsene,
jgrosseo,
jpxrk,
ktf,
mfaggin,
mmazzilli,
singhra1994,
stefanopolitano and
zhangbiao-phy
as code owners
3 participants
Clang-tidy issues present in workflows related to$\Lambda^+_c\rightarrow pK\pi^+$ reconstruction are fixed.
I am not 100% sure that some of the narrowing conversion issues are addressed in proper places (e.g. change of the Configurable
double->floatwhen it is consumed by function withfloatparameter; or issues withint8_tvalues), therefore an attentive look and feedback is appreciated.UPD: addressed (part of) issues in
Tools/KFparticle/KFUtilities.h- clang-tidy and O2 linter.